Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/sam0_common/periph: add low-level SDMMC peripheral driver for SDHC #19760

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jun 24, 2023

Contribution description

This PR implements the low-level SDIO/SDMMC peripheral driver for SAM0 SDHC according to the definition in #19539.

Testing procedure

BOARD=same54-xpro make -C tests/drivers/sdmmc
BOARD=same54-xpro make -C tests/sys/vfs_default

Issues/PRs references

Depends on PR #19539
Depends on PR #19899

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Jun 24, 2023
@gschorcht gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 24, 2023
@gschorcht gschorcht force-pushed the cpu/sam0_common/periph/sdmmc branch from ca0fa13 to 114896e Compare July 2, 2023 16:13
@github-actions github-actions bot added the Platform: ESP Platform: This PR/issue effects ESP-based platforms label Jul 2, 2023
@gschorcht gschorcht force-pushed the cpu/sam0_common/periph/sdmmc branch 2 times, most recently from 4366fb9 to dd5dded Compare July 3, 2023 05:02
@github-actions github-actions bot removed the Platform: ESP Platform: This PR/issue effects ESP-based platforms label Jul 3, 2023
bors bot added a commit that referenced this pull request Aug 23, 2023
19539: drivers/periph_sdmmc: define a High-level SDIO/SD/MMC API and low-level SDMMC periperal driver interface r=benpicco a=gschorcht

### Contribution description

This PR provides a SDIO/SD/MMC Device API (SDMMC). It implements a SD host controller driver that provides a high-level functions using a low-level SDIO/SD/MMC peripheral driver for accessing

- MultiMediaCards (MMC) and Embedded MultiMediaCards (eMMC)
- SD Memory Cards (SD Cards) with Standard Capacity (SDSC), High Capacity (SDHC) or Extended Capacity (SDXC).

It supports:

- 1-bit, 4-bit and 8-bit data bus width
- Default Speed and High Speed
- Auto-CLK

The SDIO/SD/MMC device API (SDMMC) is divided into two parts:

1. The high-level API that implements the SD Host Controller driver and allows
   - to inititialize and identify different types of cards,
   - to access them either blockwise or bytewise,
   - to get information about the used card, and
   - to send single commands or application specific commands to the card.

2. The low-level SDIO/SD/MMC peripheral driver implements the low-level functions required by the high-level device API. It has to be implemented for each MCU.

### Limitations:

- Only one card per SDIO/SD/MMC device is supported.
- eMMCs specific features are not supported.
- UHS-I, UHS-II and UHS-III are not supported.

### Testing procedure

PR #19540, PR #19760 or PR #19786 is needed to test this PR.

### Issues/PRs references

Prerequisite for PR #19540
Prerequisite for PR #19760
Prerequisite for PR #19786

19815: cpu/sam0_common/periph/sdhc: busy waiting and clock fixes r=benpicco a=benpicco



19860: drivers/ft5x06: fix vendor ID for FT6xx6 and FTxxxx register addresses r=benpicco a=gschorcht

### Contribution description

This PR provides a fix of the vendor ID for FT6xx6 touch panel driver ICs and a fix of register addresses for FTxxxx.

According to the [Application Note for FT6x06 CTPM](https://cdn-shop.adafruit.com/datasheets/FT6x06_AN_public_ver0.1.3.pdf), the vendor ID of FT6x06 touch panel driver ICs is `0x11` instead of `0xcd`. Although there are no information found in the Web about the FT6x36, the FT6336U touch panel of a ESP32-S3 WT32 SC01 Plus is also working with `0x11` as vendor ID so that it seems that FT6x36 is also using `0x11` as vendor ID.

Figured out with a `stm32f723e-disco` board (revision D03). Without this PR, `tests/drivers/ft5x06` gives:
```
+------------Initializing------------+
[ft5x06] init: invalid vendor ID: '0x11' (expected: 0xcd)
[Error] Initialization failed
```
With this PR it works as expected.
```
+------------Initializing------------+
Initialization successful
main(): This is RIOT! (Version: 2023.10-devel-96-gbb9011-drivers/ft5x06_fix_vendor_id)
FT5x06 test application

+------------Initializing------------+
[ft5x06] init: configuring touchscreen interrupt
Initialization successful
1 touch detected
[ft5x06] read gesture_id '0x00'
Touch 1 - X: 151, Y:138
[ft5x06] read gesture_id '0x00'
```

Some background information found in the Web:

- According to the [STM32CubeF7](https://github.com/STMicroelectronics/STM32CubeF7/blob/c20e6dd15bd2a90e19f28cadc703aeb26825d211/Drivers/BSP/STM32F723E-Discovery/stm32f723e_discovery_ts.c#L24-L27) the FRIDA LCD panel mounted on the `stm32f723e-disco` board either uses FT6x36 (prior revision D) or FT3x67 (revision D). However, the FT5x06 driver type for the card is defined as FT6x06, which does not seem correct: https://github.com/RIOT-OS/RIOT/blob/bb9011c3fbfe7c20bde99ce2462ef564d555ea08/boards/stm32f723e-disco/include/board.h#L59
- According to the [STM32CubeF7](https://github.com/STMicroelectronics/STM32CubeF7/blob/c20e6dd15bd2a90e19f28cadc703aeb26825d211/Drivers/BSP/Components/ft6x06/ft6x06.h#L269-L270), the vendor ID for FT6x36 should be `0xcd`. However, the FT6336U on ESP32-S3 WT32 SC01 Plus works with vendor ID `0x11`.
- The [Adafruit FT6206 library](https://github.com/adafruit/Adafruit_FT6206_Library/blob/95118cd9831890616a1afb55d323b6261dee15af/Adafruit_FT6206.h#L28) uses `0x11` as vendor id.
- The `stm32l496g-disco` board uses a FT6236 which has vendor ID `0xcd`.

So the information available on the web is confusing. Maybe, a better solution would be to accept `0x11` as well as `0xcd` as vendor ID for FT6xxx touch panels. Unfortunately, there are no documents available on the registers directly from FocalTech 😟 so it seems to be more speculation than knowledge.

### Testing procedure


### Issues/PRs references



19886: cpu/efm32: fix DAC configuration r=benpicco a=gschorcht

### Contribution description

The EFM32 MCU allows the reference voltage to be configured per DAC device, not per DAC channel. Also, the DAC reference voltage was defined in the configuration but not used anywhere.

At the moment we have only defined one board (`stwstk6220a`) that uses the DAC, so changing the configuration interface shouldn't be critical.

### Testing procedure

`tests/periph/dac` should still work for the `stwstk6220a`
```
BOARD=slwstk6220a make -j8 -C tests/periph/dac flash
```
I don't have a `stwstk6220a` board (EFM32 Series 0) so that I can't test it. I could only test it for the `sltb009a` board (EFM32 Series 1) with the change for VDAC in PR #19887.

### Issues/PRs references


Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 23, 2023
@gschorcht gschorcht force-pushed the cpu/sam0_common/periph/sdmmc branch from 1782b7c to 0497524 Compare August 23, 2023 22:40
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 23, 2023
@riot-ci
Copy link

riot-ci commented Aug 23, 2023

Murdock results

✔️ PASSED

ca44651 drivers/mtd: fix missing dependency in Kconfig for periph_sdmmc

Success Failures Total Runtime
7937 0 7937 16m:08s

Artifacts

@gschorcht gschorcht added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Sep 12, 2023
@gschorcht gschorcht force-pushed the cpu/sam0_common/periph/sdmmc branch from 0497524 to 5b6ea4c Compare September 14, 2023 11:09
@github-actions github-actions bot added the Area: drivers Area: Device drivers label Sep 14, 2023
@gschorcht gschorcht force-pushed the cpu/sam0_common/periph/sdmmc branch 2 times, most recently from e2b4dde to 8bc4934 Compare September 14, 2023 14:16
@gschorcht gschorcht removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Sep 15, 2023
bors bot added a commit that referenced this pull request Sep 19, 2023
19923: boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit r=miri64 a=gschorcht

### Contribution description

The PR adds the support for the EFM32GG11B family and the Silabs EFM32 Giant Gecko GG11 Starter Kit board.

The Silabs EFM32 Giant Gecko GG11 has the following on-board features:

- EFM32GG11B MCU with 2 MB flash and 512 kB RAM
- J-Link USB debugger
- 176x176 RGB LCD (not supported)
- 2 user buttons, 2 user RGB LEDs and a touch slider
- Si7021 Relative Humidity and Temperature Sensor
- Si7210 Hall-Effect Sensor (not supported)
- USB OTG interface (Device mode supported)
- 32 MByte Quad-SPI Flash (not supported yet)
- SD card slot (not supported yet, follow-up PR based on PR #19760)
- RJ-45 Ethernet (not supported)
- Dual microphones (not supported)

### Testing procedure

Basic tests should work.

### Issues/PRs references

19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this pull request Sep 19, 2023
19923: boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit r=miri64 a=gschorcht

### Contribution description

The PR adds the support for the EFM32GG11B family and the Silabs EFM32 Giant Gecko GG11 Starter Kit board.

The Silabs EFM32 Giant Gecko GG11 has the following on-board features:

- EFM32GG11B MCU with 2 MB flash and 512 kB RAM
- J-Link USB debugger
- 176x176 RGB LCD (not supported)
- 2 user buttons, 2 user RGB LEDs and a touch slider
- Si7021 Relative Humidity and Temperature Sensor
- Si7210 Hall-Effect Sensor (not supported)
- USB OTG interface (Device mode supported)
- 32 MByte Quad-SPI Flash (not supported yet)
- SD card slot (not supported yet, follow-up PR based on PR #19760)
- RJ-45 Ethernet (not supported)
- Dual microphones (not supported)

### Testing procedure

Basic tests should work.

### Issues/PRs references

19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

19933: examples/gcoap: add saml11-xpro to CI boards with insufficient memory r=miri64 a=krzysztof-cabaj

### Contribution description

Bors run for PR #19927 reveals that ```examples\gcoap``` do not fit in saml11-xpro board. 
I checked and this same linker error also appears on current master branch. 

This PR add sampl11-xpro board to the Makefile.ci BOARD_INSUFFICIENT_MEMORY list.

### Testing procedure

See logs from bors https://ci.riot-os.org/details/073c5dadc6ba4bc8a613edb78a1a4a2d

### Issues/PRs references

PR #19927 


Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this pull request Sep 19, 2023
19923: boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit r=miri64 a=gschorcht

### Contribution description

The PR adds the support for the EFM32GG11B family and the Silabs EFM32 Giant Gecko GG11 Starter Kit board.

The Silabs EFM32 Giant Gecko GG11 has the following on-board features:

- EFM32GG11B MCU with 2 MB flash and 512 kB RAM
- J-Link USB debugger
- 176x176 RGB LCD (not supported)
- 2 user buttons, 2 user RGB LEDs and a touch slider
- Si7021 Relative Humidity and Temperature Sensor
- Si7210 Hall-Effect Sensor (not supported)
- USB OTG interface (Device mode supported)
- 32 MByte Quad-SPI Flash (not supported yet)
- SD card slot (not supported yet, follow-up PR based on PR #19760)
- RJ-45 Ethernet (not supported)
- Dual microphones (not supported)

### Testing procedure

Basic tests should work.

### Issues/PRs references

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and works as expected.
Not sure where the extra 2k ROM compared to sam0_sdhc come from, but I'm happy about shared code - please squash!

@gschorcht
Copy link
Contributor Author

Looks good to me and works as expected.

It would be interesting whether it works as stable as the busy waiting implementation of sam0_sdhc. Perhaps we should allow the use of both implementations for a while. In the PR, enabling the sam0_sdhc module will actually cause the sdmmc_sdhc module to be used.

Not sure where the extra 2k ROM compared to sam0_sdhc come from,

The implementation differs completely. The increase is probably caused by both sdmmc and sdmmc_sdhc.

At this point, the question arises whether a copyright notice of "Copyright (C) 2018 Alkgrove" in sdmmc_sdhc is still necessary. diff says that 954 lines of the 1181 of sam0_sdhc.c have been changed. A lot lines in sam0_sdhc.c were already modifications.

@benpicco
Copy link
Contributor

Perhaps we should allow the use of both implementations for a while.

Yea I think we can start by deprecating the sam0_sdhc after the next release, we will only find issues by using the new module for a while.

Existing (externa) boards will keep using the old module unless they opt into the new implementation for the time being.

At this point, the question arises whether a copyright notice of "Copyright (C) 2018 Alkgrove" in sdmmc_sdhc is still necessary

If you think it's a rewrite at this point, feel free to drop it. You could also write that it's based on the driver from Alkgrove or that it took inspiration from it if that's the case.

@gschorcht
Copy link
Contributor Author

At this point, the question arises whether a copyright notice of "Copyright (C) 2018 Alkgrove" in sdmmc_sdhc is still necessary

If you think it's a rewrite at this point, feel free to drop it. You could also write that it's based on the driver from Alkgrove or that it took inspiration from it if that's the case.

I have checked again. To be honest, the new code looks rather like a rewrite than a modification. Neither the structure nor the code seems like a simple modification of https://github.com/alkgrove/initmaker/blob/master/samd5x/src/sd.c. The reason is probably that you already changed the code a lot and I took some code from your implementation and modified it again a lot to implement the API functions. Of course there are some small code snippets that are identical, e.g.

    while (!sdhc->CCR.bit.INTCLKS) {}    /* wait for clock to be stable */

but there is no usual way to implement it and is trivial.

@benpicco
Copy link
Contributor

Please squash!

@gschorcht gschorcht force-pushed the cpu/sam0_common/periph/sdmmc branch from 4b33890 to 4cda0e4 Compare September 23, 2023 17:26
@benpicco
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Sep 24, 2023
19760: cpu/sam0_common/periph: add low-level SDMMC peripheral driver for SDHC r=benpicco a=gschorcht

### Contribution description

This PR implements the low-level SDIO/SDMMC peripheral driver for SAM0 SDHC according to the definition in #19539.

### Testing procedure

```
BOARD=same54-xpro make -C tests/drivers/sdmmc
```
```
BOARD=same54-xpro make -C tests/sys/vfs_default
```

### Issues/PRs references

~Depends on PR #19539~
Depends on PR #19899

19942: pkg/littlefs2: bump to v2.8 r=benpicco a=benpicco




Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@bors
Copy link
Contributor

bors bot commented Sep 24, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Sep 24, 2023
19760: cpu/sam0_common/periph: add low-level SDMMC peripheral driver for SDHC r=benpicco a=gschorcht

### Contribution description

This PR implements the low-level SDIO/SDMMC peripheral driver for SAM0 SDHC according to the definition in #19539.

### Testing procedure

```
BOARD=same54-xpro make -C tests/drivers/sdmmc
```
```
BOARD=same54-xpro make -C tests/sys/vfs_default
```

### Issues/PRs references

~Depends on PR #19539~
Depends on PR #19899

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
@bors
Copy link
Contributor

bors bot commented Sep 24, 2023

Build failed:

@benpicco
Copy link
Contributor

benpicco commented Sep 28, 2023

It's always Kconfig 😕

Enabling `ztimer_msec` through `sdmmc` in case `ztimer_usec` is not enabled will not work if the dependencies for `sdmmc` are resolved before the dependencies of another module that enables `ztimer_usec`. Therefore, `ztimer_msec` has to be enabled by `sdmmc`
@gschorcht
Copy link
Contributor Author

It's always Kconfig 😕

Commits 5393c75 and ca44651 should fix it.

  1. Implying sdmmc if module mtd and the board has the periph_sdmmc feature was missing (ca44651).
  2. Enabling ztimer_msec by sdmmc in case ztimer_usec is not enabled will not work if the dependencies for sdmmc are resolved before the dependencies of another module that enables ztimer_usec. Therefore, ztimer_msec has to be enabled by sdmmc.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Sep 29, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 149cee4 into RIOT-OS:master Sep 29, 2023
23 checks passed
Comment on lines +804 to +808
/* TODO timeout handling */
if (sdhc->CCR.bit.SDCLKEN) {
DEBUG("[sdmmc] disable SDCLK\n");
/* wait for command/data to go inactive */
while (sdhc->PSR.reg & (SDHC_PSR_CMDINHC | SDHC_PSR_CMDINHD)) {}
Copy link
Contributor

@benpicco benpicco Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I really do see it getting stuck here when I run tests/sys/vfs_default with #20053 and

USEMODULE += lwext4_vfs
CFLAGS += -DCONFIG_MTD_SDMMC_ERASE=1 # not sure if this is really needed
CFLAGS += -DTHREAD_STACKSIZE_MAIN=THREAD_STACKSIZE_LARGE

The test will start by formatting the SD card (which seems to be done rather inefficiently, there are many duplicate writes it seems) but this onslaught of SD commands does a good job of triggering the bug.

I tried adding a naive timeout:

--- a/drivers/sdmmc/sdmmc_sdhc.c
+++ b/drivers/sdmmc/sdmmc_sdhc.c
@@ -805,7 +805,14 @@ static int _disable_sd_clk(sdhc_dev_t *sdhc_dev)
     if (sdhc->CCR.bit.SDCLKEN) {
         DEBUG("[sdmmc] disable SDCLK\n");
         /* wait for command/data to go inactive */
-        while (sdhc->PSR.reg & (SDHC_PSR_CMDINHC | SDHC_PSR_CMDINHD)) {}
+        uint32_t timeout = 0xfffff;
+        while (sdhc->PSR.reg & (SDHC_PSR_CMDINHC | SDHC_PSR_CMDINHD)) {
+            if (--timeout == 0) {
+                break;
+            }
+        }
+        printf("cmd took %lu loops\n", 0xfffff - timeout);
+
         /* disable the clock to card */
         sdhc->CCR.reg &= ~SDHC_CCR_SDCLKEN;
     }

but the controller does not recover from this condition:

2023-11-07 19:02:20,637 - INFO # mtd_sdmmc_write_page: page:442384 offset:0 size:1024
2023-11-07 19:02:20,643 - INFO # cmd took 0 loops
2023-11-07 19:02:20,645 - INFO # write bgroup 36 / 948
2023-11-07 19:02:20,649 - INFO # lwext4: erase 2 sectors starting with lu
2023-11-07 19:02:20,652 - INFO # cmd took 0 loops
2023-11-07 19:02:20,655 - INFO # lwext4: write 1024 bytes to page 458766
2023-11-07 19:02:20,660 - INFO # mtd_sdmmc_write_page: page:458766 offset:0 size:1024
2023-11-07 19:02:20,666 - INFO # cmd took 0 loops
2023-11-07 19:02:20,667 - INFO # write bgroup 37 / 948
2023-11-07 19:02:20,671 - INFO # lwext4: erase 2 sectors starting with lu
2023-11-07 19:02:22,980 - INFO # cmd took 1048575 loops
2023-11-07 19:02:22,984 - INFO # lwext4: write 1024 bytes to page 475150
2023-11-07 19:02:22,988 - INFO # mtd_sdmmc_write_page: page:475150 offset:0 size:1024
2023-11-07 19:02:23,490 - INFO # [sdmmc] Card not present
2023-11-07 19:02:23,493 - INFO # mtd_sdmmc_write_page: error -19

@MrKevinWeiss MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023
@gschorcht gschorcht deleted the cpu/sam0_common/periph/sdmmc branch December 6, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants